Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added tampered buffer deserialization tests #174

Open
wants to merge 2 commits into
base: rolling
Choose a base branch
from

Conversation

MiguelCompany
Copy link
Contributor

Add tests for deserialization of a corrupted buffer.

The message type has been chosen on purpose to be UnboundedSequences in order to check the fixes on ros2/rmw_fastrtps#505 and eProsima/Fast-CDR#98.

They perform the following sequence:

  1. Serialization of a default constructed UnboundedSequences message, this would produce a buffer with a lot of zero bytes indicating the length of each sequence, which are 0.
  2. Modify the resulting buffer to all FF's. This may produce crashes as they will be interpreted as sequences of 2^32 - 1 elements and either allocation errors or access beyond the end of the serialized buffer may occur
  3. Expect the deserialization to fail

These tests fail with rmw_fastrtps_cpp and rmw_fastrtps_dynamic_cpp unless Fast CDR is updated to v1.0.19 and ros2/rmw_fastrtps#505 is applied

These new tests fail locally on rmw_cyclonedds, so it may need to be updated

@MiguelCompany
Copy link
Contributor Author

@eboasson FYI, it seems there is an issue on rmw_cyclonedds with the deserialization of corrupted buffers.

Signed-off-by: Miguel Company <[email protected]>
@eboasson
Copy link

eboasson commented Feb 3, 2021

@eboasson FYI, it seems there is an issue on rmw_cyclonedds with the deserialization of corrupted buffers.

Thanks, yes, the ROS2-specific deserialiser is a bit cavalier. The planned clean up still hasn't happened.

@clalancette clalancette self-assigned this Feb 18, 2021
@audrow audrow changed the base branch from master to rolling June 28, 2022 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants